Pass a custom error for invalid WP.com email errors #652
Conversation
…ors from `GetStartedViewController`.
itsmeichigo
left a comment
itsmeichigo
left a comment
There was a problem hiding this comment.
I understand that the new error type is needed to avoid breaking changes (compared to updating the delegate method handleError with a new source parameter), and this works with the integration in Woo, so I'm approving this. Leaving a few nit-picking comments regarding the other case for source below though, please consider it if you think it makes sense.
| private func presentGetStartedView() { | ||
| guard let toVC = GetStartedViewController.instantiate(from: .getStarted) else { | ||
| DDLogError("Failed to navigate to GetStartedViewController") | ||
| return | ||
| } | ||
|
|
||
| navigationController?.pushViewController(toVC, animated: true) | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder why this is not needed anymore?
There was a problem hiding this comment.
I forgot to make a note on this, this is dead code 😅 I was checking where GetStartedViewController is initialized in WPAuthenticator, and thought it's good to remove any unused code.
| /// Other unspecified sources. | ||
| case other |
There was a problem hiding this comment.
Nit: I'm thinking this case is not really necessary, since we want to be as specific as possible in enums. If we want to avoid breaking changes, when integrating this library we could use @unknown default case to handle new cases in the future. WDYT?
There was a problem hiding this comment.
Yea, I think it makes sense to not include this default case and only errors with specific sources are passed to the client app. I made an update in 8591d33
| // This is public so it can be set from StoredCredentialsAuthenticator. | ||
| var errorMessage: String? | ||
|
|
||
| var source: SignInSource = .other |
There was a problem hiding this comment.
Since I suggested removing other case above, maybe we should keep source optional here 🤔.
Yes 😅 I was debating whether to make a breaking change for a better solution or a more implicit change with the error type as in this PR. Since we're on a tight schedule in Q3, I went with the latter one. |
For woocommerce/woocommerce-ios#7318
Description
In order to track the source of invalid email errors for WCiOS local notifications experiment, we need a way to differentiate the two flows - "Enter Your Store Address" or "Continue With WordPress.com". This PR added a new
SignInErrorandSignInSourceand parsed the error for invalid email based on the WCiOS implementation.WPiOS does not handle errors in a custom way like WCiOS.
Testing steps
Please refer to the WCiOS draft PR to test the integration.